Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLI] Add c2json #8817

Merged
merged 16 commits into from
Oct 7, 2020
Merged

[CLI] Add c2json #8817

merged 16 commits into from
Oct 7, 2020

Conversation

Erovia
Copy link
Member

@Erovia Erovia commented Apr 15, 2020

Description

First of all, I'd like to thank both @lf- for starting to work on this feature in #7218 and @skullydazed for suggesting a Pygments-based approach.

This submodules attemps to parse a keymap.c file and generate a keymap.json for the user.
Sadly this is not as easy as the other way around, so parsing is limited. Sorry drashna. :)

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

This PR intends to supersede both

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@Erovia Erovia requested review from skullydazed and a team April 15, 2020 23:08
@Erovia Erovia added cli qmk cli command python labels Apr 16, 2020
@Erovia
Copy link
Member Author

Erovia commented Apr 16, 2020

CLI CI fails because qmkfm/base_container has old qmk.

@Erovia Erovia force-pushed the cli/c2json branch 2 times, most recently from fffa2c4 to 83c2e39 Compare April 19, 2020 08:39
@fastjames
Copy link

This feature would be a huge benefit to me -- I'm fairly new to QMK and I have trouble keeping my layouts straight in my head, especially when I'm making small changes. @Erovia is there any specific task I could help out with here? I'm not super-familiar with travis / docker but if you have any more insight on what's failing I am happy to dive in as deep as I can.

@Erovia
Copy link
Member Author

Erovia commented Jul 2, 2020

Thank you for your interest!
This feature is still on my TODO list, so it's by no means forgotten, just life took priority.

I'll need to rebase and fix the conflicts. Also, I think this feature could benefit greatly from this PR, but I think I'd rather see this merged as is and improve it iteratively.

Ultimately, this just needs testing. (and the rebasing I mentioned)

@fastjames
Copy link

Thanks for the fast follow-up -- I definitely understand the priority. I'll try using this branch locally with my own keymap work, to see what results I get.

@Erovia
Copy link
Member Author

Erovia commented Jul 2, 2020

That would be great.
I'll try to do the rebase today, no promises though. :)

@Erovia Erovia force-pushed the cli/c2json branch 2 times, most recently from 4064a26 to 1b44d5a Compare July 2, 2020 19:14
@Erovia
Copy link
Member Author

Erovia commented Jul 2, 2020

@fastjames Rebased and fixed up the code. If you have some time to try it out, I'd appreciate the feedback.

@fastjames
Copy link

Thanks very much for freshening this up. I had a few minutes today so here's the test procedure I followed:

  1. Check out your branch (cli/c2json)
  2. cherry-pick in my commit with my custom crkbd/rev1 keymap
  3. bin/qmk c2json -kb crkbd/rev1 --no-cpp keyboards/crkbd/keymaps/fastjames/keymap.c > ~/fj-crkbd.json
  4. Visit https://config.qmk.fm/#/crkbd/rev1/LAYOUT
  5. Click on Import QMK Keymap JSON file
  6. Select the file generated in step 3
  7. Observe results

Off the bat, I got a generic alert stating Sorry, this doesn't appear to be a QMK keymap file. Since I didn't have a good starting place for troubleshooting, I exported the default crkbd layout (using the handy nearby button) and I compared that to the c2json generated file. I noted a couple of potentially significant differences:

  1. The c2json-generated file had spaces between the key-value separators and the values. I don't know if it's a dealbreaker, but the default crkbd file that I downloaded did not have them.
  2. The c2json-generated file was missing the following JSON elements: version, notes, documentation, and keymap. I wasn't sure if any of these were significant so I started adding them to the generated file one at a time. The one that seemed to make a difference was keymap. Adding that got me past the previously described error alert.

I think I hit success after that! I can see the keymap in the configurator, and the layers appear to match what I recall setting up. I hope this testing information helps, and I'm happy to take a shot at making the changes if you're busy.

@elfmimi
Copy link
Contributor

elfmimi commented Jul 18, 2020

I tried c2json for my keymap for ARM target .
I knew that ARM does not need PROGMEM attribute so I omitted it when I wrote that keymap.
In this case c2json fails with unhelpful message: "Something went wrong" .

@ymotongpoo
Copy link

Mentions #6877 here for visibility.

Thank you @Erovia for creating this pull request. This is what I long for in qmk command. @skullydazed can we help anything to move this PR forward?

@Erovia
Copy link
Member Author

Erovia commented Sep 11, 2020

Rebased and fixed the missing keymap value. Thanks @fastjames for the awesome comment/bug-report! It helped a great deal!

@elfmimi: For the time being, I'd rather not complicate the parsing with edge-cases like the missing PROGMEM. In the future me or someone else might get back to it.

@ymotongpoo

Sorry for the ping, but thought you guys might like to know that I'm still working on this :)

@Buckwich
Copy link
Contributor

@Erovia Thank you so much for your work, I used this to build a list of all keymaps (https://github.com/qmk-helper/qmk-database) which is used for a keyboard/keymap browser. Because of this I am hoping this PR gets merged soon so I can automate the generation of JSONs.

Another issue besides the keymap field are layer modifiers with integers as identifier MO(1) due to the c processor, which are required for a correct function of the QMK configurator. I am no expert with lex but I think I solved this issue with these lines:

        else:        
            if is_adv_kc:
                layer['keycodes'][-1] += line[1]
            elif line[0] is Token.Literal.Number.Integer and is_keymap:              
                if not layer['name']:
                    layer['name'] = line[1]

One example keymap with this issue is thevankeyboards/minivan:default

keymap.c (simplified):

#define _QW 0
#define _L1 3
const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
  [_QW] = LAYOUT(   
    MO(_L1)
  )
}

keymap.c preprocessed:

const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
  [0] = LAYOUT(   
    MO(3)
  )
}

keymap.json, no-cpp, correct

{"layers": [["MO(_L1)"]]}

keymap.json, with cpp, actual result

{"layers": [["MO()"]]}

keymap.json, with cpp, expected result

{"layers": [["MO(3)"]]}

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!
@Erovia
Copy link
Member Author

Erovia commented Sep 14, 2020

@Buckwich thanks for giving this subcommand a proper test-drive and use-case! :D
Double-thanks for the bug report and for even providing a fix.

With ddacab8 those pesky numbers should be handled. Can you give it a spin?

@Buckwich
Copy link
Contributor

Looks good to me, only the position of the keymap name moved in the json (to the correct position). Numbers are working. Thank you for the quick fix.

lib/python/qmk/commands.py Outdated Show resolved Hide resolved
lib/python/qmk/commands.py Outdated Show resolved Hide resolved
lib/python/qmk/keymap.py Outdated Show resolved Hide resolved
lib/python/qmk/tests/test_cli_commands.py Outdated Show resolved Hide resolved
lib/python/qmk/tests/test_cli_commands.py Outdated Show resolved Hide resolved
@skullydazed skullydazed merged commit 058737f into qmk:master Oct 7, 2020
@ymotongpoo
Copy link

woohoo!! Thank you @Erovia, @Buckwich and @skullydazed for merging this into master!

Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Oct 7, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (178 commits)
  Docs: fix udev rules
  [CLI] Add c2json (qmk#8817)
  Improve LAYOUT macro searching (qmk#9530)
  CLI: update subcommands to use return instead of exit() (qmk#10323)
  Fixes small typo in docs (qmk#10515)
  Update personal keymap for Let's Split keyboard. (qmk#10536)
  [Keymap] Move my custom functions and keymaps to userspace (qmk#10502)
  [Keyboard] add support for ymd75 rev3 (qmk#10483)
  [Keyboard] Add soy20 PCB (qmk#10440)
  Fix for MIDI sustain effect issue (qmk#10361)
  format code according to conventions [skip ci]
  [Keyboard] Add Yugo-M Controller (qmk#10389)
  [Keymap] Add onekey keymap for OLED testing (qmk#10380)
  [Keymap] Add winterNebs keymaps  (qmk#10328)
  [Keymap] Added 333fred 5x6_5 keymap (qmk#10272)
  [Keyboard] Add hannah60rgb rev.2 PCB (qmk#10287)
  Adding VIA support to katana60 rev2 (qmk#10442)
  OLED driver fixes (qmk#10377)
  IS31FL3741 driver fixup (qmk#10519)
  add info.json for XD75 keyboard (qmk#10523)
  ...
barrettclark pushed a commit to barrettclark/qmk_firmware that referenced this pull request Oct 8, 2020
* upstream/master: (81 commits)
  [Keyboard] New keyboard - eiri (qmk#10529)
  [Keymap] Add niu mini dye sub keymap (qmk#10525)
  Clean ChibiOS platform files (qmk#10505)
  [Keyboard] LeftyNumpad Keyboard (qmk#10500)
  [Keyboard] add maja capslock indicator (qmk#10151)
  Fix issue introduced by PR#10404 (qmk#10559)
  Docs: fix udev rules
  [CLI] Add c2json (qmk#8817)
  Improve LAYOUT macro searching (qmk#9530)
  CLI: update subcommands to use return instead of exit() (qmk#10323)
  Fixes small typo in docs (qmk#10515)
  Update personal keymap for Let's Split keyboard. (qmk#10536)
  [Keymap] Move my custom functions and keymaps to userspace (qmk#10502)
  [Keyboard] add support for ymd75 rev3 (qmk#10483)
  [Keyboard] Add soy20 PCB (qmk#10440)
  Fix for MIDI sustain effect issue (qmk#10361)
  format code according to conventions [skip ci]
  [Keyboard] Add Yugo-M Controller (qmk#10389)
  [Keymap] Add onekey keymap for OLED testing (qmk#10380)
  [Keymap] Add winterNebs keymaps  (qmk#10328)
  ...
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
* Basic keymap parsing finally works

* Add 'keymap.json' creation to the qmk.keymap module

* Add tests and fix formatting

* Fix/exclude flake8 errors

* Convert keymap.c to valid keymap.json

* Fix some errors

* Add tests

* Finalize keymap.json creation, add json template

* Add docs

* Move pygments to the standard requirements

* Add support for nameless layers, fix tests

* Fix things after rebase

* Add missing 'keymap' value.

* Fix missing layer numbers from advanced keycodes

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!

* Apply suggestions from code review

* fixup tests

Co-authored-by: Zach White <[email protected]>
Co-authored-by: skullY <[email protected]>
oscarcarlsson pushed a commit to oscarcarlsson/qmk_firmware that referenced this pull request Nov 2, 2020
* Basic keymap parsing finally works

* Add 'keymap.json' creation to the qmk.keymap module

* Add tests and fix formatting

* Fix/exclude flake8 errors

* Convert keymap.c to valid keymap.json

* Fix some errors

* Add tests

* Finalize keymap.json creation, add json template

* Add docs

* Move pygments to the standard requirements

* Add support for nameless layers, fix tests

* Fix things after rebase

* Add missing 'keymap' value.

* Fix missing layer numbers from advanced keycodes

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!

* Apply suggestions from code review

* fixup tests

Co-authored-by: Zach White <[email protected]>
Co-authored-by: skullY <[email protected]>
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Nov 24, 2020
* Basic keymap parsing finally works

* Add 'keymap.json' creation to the qmk.keymap module

* Add tests and fix formatting

* Fix/exclude flake8 errors

* Convert keymap.c to valid keymap.json

* Fix some errors

* Add tests

* Finalize keymap.json creation, add json template

* Add docs

* Move pygments to the standard requirements

* Add support for nameless layers, fix tests

* Fix things after rebase

* Add missing 'keymap' value.

* Fix missing layer numbers from advanced keycodes

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!

* Apply suggestions from code review

* fixup tests

Co-authored-by: Zach White <[email protected]>
Co-authored-by: skullY <[email protected]>
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Jan 13, 2021
* Basic keymap parsing finally works

* Add 'keymap.json' creation to the qmk.keymap module

* Add tests and fix formatting

* Fix/exclude flake8 errors

* Convert keymap.c to valid keymap.json

* Fix some errors

* Add tests

* Finalize keymap.json creation, add json template

* Add docs

* Move pygments to the standard requirements

* Add support for nameless layers, fix tests

* Fix things after rebase

* Add missing 'keymap' value.

* Fix missing layer numbers from advanced keycodes

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!

* Apply suggestions from code review

* fixup tests

Co-authored-by: Zach White <[email protected]>
Co-authored-by: skullY <[email protected]>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Basic keymap parsing finally works

* Add 'keymap.json' creation to the qmk.keymap module

* Add tests and fix formatting

* Fix/exclude flake8 errors

* Convert keymap.c to valid keymap.json

* Fix some errors

* Add tests

* Finalize keymap.json creation, add json template

* Add docs

* Move pygments to the standard requirements

* Add support for nameless layers, fix tests

* Fix things after rebase

* Add missing 'keymap' value.

* Fix missing layer numbers from advanced keycodes

Buckwich noticed that if the advanced keycode / layer toggling key
contains a number, it goes missing.
Now we properly handle them.
Thx for noticing!

* Apply suggestions from code review

* fixup tests

Co-authored-by: Zach White <[email protected]>
Co-authored-by: skullY <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants